Skip to content

EIP 7742 implementation#7518

Open
yerke26 wants to merge 84 commits intomasterfrom
feature/eip-7742
Open

EIP 7742 implementation#7518
yerke26 wants to merge 84 commits intomasterfrom
feature/eip-7742

Conversation

@yerke26
Copy link
Contributor

@yerke26 yerke26 commented Sep 30, 2024 •
edited
Loading

Resolves #7526

Changes

removed MAX_BLOB_GAS_PER_BLOCK, TARGET_BLOB_GAS_PER_BLOCK
Get target_blob_count, max_blob_count from Engine Api
added target_blob_count, max_blob_count to ExecutionPayload
added target_blob_count, max_blob_count to BlockHeader
removed verifications related to MAX_BLOB_GAS_PER_BLOCK
used target_blob_count * Eip4844Constants.GasPerBlob instead of TARGET_BLOB_GAS_PER_BLOCK

Types of changes

What types of changes does your code introduce?

  • Bugfix (a non-breaking change that fixes an issue)
  • New feature (a non-breaking change that adds functionality)
  • Breaking change (a change that causes existing functionality not to work as expected)
  • Optimization
  • Refactoring
  • Documentation update
  • Build-related changes
  • Other: Description

Testing

Requires testing

  • Yes
  • No

If yes, did you write tests?

  • Yes
  • No

Notes on testing

Optional. Remove if not applicable.

Documentation

Requires documentation update

  • Yes
  • No

If yes, link the PR to the docs update or the issue with the details labeled docs. Remove if not applicable.

Requires explanation in Release Notes

  • Yes
  • No

If yes, fill in the details here. Remove if not applicable.

Remarks

Optional. Remove if not applicable.

@yerke26 yerke26 requested a review from rubo as a code owner September 30, 2024 14:33
@yerke26 yerke26 marked this pull request as draft September 30, 2024 14:33
MarekM25
MarekM25 requested changes Sep 30, 2024
View reviewed changes
Copy link
Contributor

@MarekM25 MarekM25 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

very quick check: no HeaderDecoder changes?

@yerke26 yerke26 changed the title Get target_blob_count, max_blob_count from Engine Api EIP 7742 implementation Oct 1, 2024
@yerke26 yerke26 marked this pull request as ready for review October 2, 2024 10:15
yerke26 added 2 commits October 2, 2024 17:21
@yerke26
add tests
cc02ce5
@yerke26
Merge branch 'master' into feature/eip-7742
206d514 
# Conflicts:
#	src/Nethermind/Nethermind.JsonRpc/Modules/Eth/FeeHistory/FeeHistoryOracle.cs
MarekM25
MarekM25 requested changes Oct 3, 2024
View reviewed changes
Copy link
Contributor

@MarekM25 MarekM25 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Red flag, don't see the change in PrepareBlockForProcessing in BlockchainProcessor. Perhaps, check how other fields from header are used

yerke26 added 13 commits October 4, 2024 15:41
@yerke26
add missing fields
9fbb9c1
@yerke26
fix PayloadAttributes
8ada4e2
@yerke26
add engine_forkchoiceUpdatedV4 to EngineRpcModule.Prague
3ae0fed
@yerke26
change to engine_forkchoiceUpdatedV4
372868a
@yerke26
Merge branch 'master' into feature/eip-7742
0bd75c6
@yerke26
add new fields to EngineModuleTests.V4
4b5fbfc
@yerke26
added new engine_forkchoiceUpdatedV4 to EngineRpcCapabilitiesProvider
5078b76
@yerke26
fix test cases
a92cdde
@yerke26
Merge branch 'master' into feature/eip-7742
c032480 
# Conflicts:
#	src/Nethermind/Nethermind.Core/Specs/IReleaseSpec.cs
#	src/Nethermind/Nethermind.Merge.Plugin/Data/ExecutionPayloadV4.cs
#	src/Nethermind/Nethermind.Merge.Plugin/Handlers/EngineRpcCapabilitiesProvider.cs
@yerke26
fix whitespace
4ab5fda
@yerke26
fix EngineRpcCapabilitiesProvider, revert ChainSpecBasedSpecProviderT…
0ba2006 
…ests
@yerke26
remove unnecessary comment
911d232
@yerke26
remove comments
f806f22
LukaszRozmej
LukaszRozmej reviewed Dec 9, 2024
View reviewed changes
LukaszRozmej
LukaszRozmej reviewed Dec 9, 2024
View reviewed changes
tanishqjasoria
tanishqjasoria reviewed Dec 10, 2024
View reviewed changes
LukaszRozmej
LukaszRozmej approved these changes Dec 10, 2024
View reviewed changes
Copy link
Member

@LukaszRozmej LukaszRozmej left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Apart of MaxBlobCount confusion I am fine with the PR

@yerke26 @LukaszRozmej
Update src/Nethermind/Nethermind.Consensus/Producers/TxPoolTxSource.cs
2eb9fee 
Co-authored-by: Lukasz Rozmej <lukasz.rozmej@gmail.com>
tanishqjasoria
tanishqjasoria suggested changes Dec 11, 2024
View reviewed changes
yerke26 and others added 2 commits December 11, 2024 15:23
@yerke26 @LukaszRozmej
Update src/Nethermind/Nethermind.JsonRpc/Modules/Eth/FeeHistory/FeeHi…
6475265 
…storyOracle.cs

Co-authored-by: Lukasz Rozmej <lukasz.rozmej@gmail.com>
@yerke26
fix whitespaces
dfe634e
ak88
ak88 reviewed Dec 11, 2024
View reviewed changes
Copy link
Contributor

@ak88 ak88 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what about the new way to calculate excess blob gas that the EIP mentions?
https://eips.ethereum.org/EIPS/eip-7742#block-processing

src/Nethermind/Nethermind.Consensus/Producers/TxPoolTxSource.cs

if (!spec.IsEip7742Enabled || payloadAttributes?.MaxBlobCount is not null)
{
SelectBlobTransactions(blobTransactions, parent, spec, selectedBlobTxs);
Copy link
Contributor

@ak88 ak88 Dec 11, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You dont pass the PayloadAttributes

src/Nethermind/Nethermind.Consensus/Producers/TxPoolTxSource.cs
}

private void SelectBlobTransactions(IEnumerable<Transaction> blobTransactions, BlockHeader parent, IReleaseSpec spec, ArrayPoolList<Transaction> selectedBlobTxs)
private void SelectBlobTransactions(IEnumerable<Transaction> blobTransactions, BlockHeader parent, IReleaseSpec spec, ArrayPoolList<Transaction> selectedBlobTxs, PayloadAttributes? payloadAttributes = null)
Copy link
Contributor

@ak88 ak88 Dec 11, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why is PayloadAttributes optional?

@yerke26
Merge branch 'master' into feature/eip-7742
e231959 
# Conflicts:
#	src/Nethermind/Nethermind.Merge.AuRa.Test/AuRaMergeEngineModuleTests.cs
#	src/Nethermind/Nethermind.Merge.Plugin.Test/EngineModuleTests.V4.cs
@yerke26
resolve merge conflicts
5c6dc2c
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ak88 ak88 ak88 left review comments

@LukaszRozmej LukaszRozmej LukaszRozmej approved these changes

@rubo rubo Awaiting requested review from rubo rubo is a code owner

@MarekM25 MarekM25 Awaiting requested review from MarekM25 MarekM25 is a code owner

+1 more reviewer

@tanishqjasoria tanishqjasoria tanishqjasoria approved these changes

Reviewers whose approvals may not affect merge requirements

Requested changes must be addressed to merge this pull request.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Implement EIP-7442 Uncouple blob count between CL and EL

5 participants

@yerke26 @tanishqjasoria @LukaszRozmej @ak88 @MarekM25